Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use alpha for stake in neuron info #888

Closed
wants to merge 3 commits into from
Closed

Use alpha for stake in neuron info #888

wants to merge 3 commits into from

Conversation

ales-otf
Copy link
Contributor

@ales-otf ales-otf commented Oct 30, 2024 •
edited
Loading

Description

This PR fixes inconsistency issues related to staking values returned by RPC's NeuronInfoRuntimeApi and StakeInfoRuntimeApi.

Getters for the stake values are added to NeuronInfo, NeuronInfoLite and StakeInfo, because there is no other way to test this values in the integration tests.

Related Issue(s)

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Other (please describe):

Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have run cargo fmt and cargo clippy to ensure my code is formatted and linted correctly
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Additional Notes

It would be better to introduce different types for TAO and Alpha balances, so these kinds of errors would be discoverable at the compilation time (#887).

Also, to prevent possible inconsistencies between NeuronInfo and NeuronInfoLite it would be better to make NeuronInfoLite a part of NeuronInfo, instead of duplicating information between them.

Allowing unit/module tests would help with private API testing. In case the module tests are not possible, another option could be a cargo feature, which would make API dedicated to tests being compiled for tests only (#[cfg(test)] doesn't work for the integration tests).

@ales-otf ales-otf marked this pull request as ready for review October 30, 2024 19:20
@ales-otf ales-otf requested a review from unconst as a code owner October 30, 2024 19:20
@ales-otf ales-otf self-assigned this Oct 30, 2024
@camfairchild
Copy link
Contributor

The reason we use stake_weight is because this is what miners need to prioritize Validator requests appropriately.
This value directly impacts a miner's emission, versus the alpha entry, which is not meaningful.

@ales-otf
Copy link
Contributor Author

ales-otf commented Oct 30, 2024 •
edited
Loading

The reason we use stake_weight is because this is what miners need to prioritize Validator requests appropriately. This value directly impacts a miner's emission, versus the alpha entry, which is not meaningful.

But in NeuronInfoLite the value was from Stake storage, while in NeuronInfo from StakeWeight, and in StakeInfo it's Alpha. Which one is true then?

Or is it StakeWeight only in NeuronInfo?

@ales-otf
Copy link
Contributor Author

The reason we use stake_weight is because this is what miners need to prioritize Validator requests appropriately. This value directly impacts a miner's emission, versus the alpha entry, which is not meaningful.

Can we add additional field stake_weight to NeuronInfo and NeuronInfoLite? Also, if we can, could we make NeuronInfoLite being a part of NeuronInfo to omit the duplication and inconsistencies?

@sam0x17 sam0x17 added the skip-cargo-audit This PR fails cargo audit but needs to be merged anyway label Oct 31, 2024
@ales-otf ales-otf force-pushed the fix/rpc-stake-values branch from 5dcb332 to 9952d4e Compare October 31, 2024 14:29
@camfairchild
Copy link
Contributor

The reason we use stake_weight is because this is what miners need to prioritize Validator requests appropriately. This value directly impacts a miner's emission, versus the alpha entry, which is not meaningful.

But in NeuronInfoLite the value was from Stake storage, while in NeuronInfo from StakeWeight, and in StakeInfo it's Alpha. Which one is true then?

Or is it StakeWeight only in NeuronInfo?

I believe this change is attempting to keep the same struct type-wise, while changing the value to be as meaningful to the client as it was.

When we didn't have the Alpha map, the only value that mattered in this context was the Stake map. But now we use a calculated stake_weight to mean the same thing. So this is what should be returned

@camfairchild
Copy link
Contributor

Can we add additional field stake_weight to NeuronInfo and NeuronInfoLite?

We could, but the big issue with this is we don't have proper versioning set up for the RuntimeAPIs and RPCs. So this will break any clients that rely on this API (many).

@gztensor
Copy link
Contributor

@camfairchild , we have get_global_for_hotkey_and_coldkey and get_global_for_hotkey_and_coldey_on_subnet methods. Can we just use them in StakeInfo and NeuronInfo respectively?

@ales-otf ales-otf mentioned this pull request Oct 31, 2024
Closed
13 tasks
@ales-otf
Copy link
Contributor Author

ales-otf commented Oct 31, 2024 •
edited
Loading

Closing, as NeuronInfo::stake should be StakeWeight and the difference between StakeInfo::stake and NeuronInfo::stake is expected and should be handled on the client's side.

To fix the inconsistency between NeuronInfo::stake and NeuronInfoLite::stake a separate PR is open.

@ales-otf ales-otf closed this Oct 31, 2024
@ales-otf ales-otf deleted the fix/rpc-stake-values branch October 31, 2024 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@unconst unconst Awaiting requested review from unconst unconst is a code owner

Assignees

@ales-otf ales-otf

Labels
skip-cargo-audit This PR fails cargo audit but needs to be merged anyway
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ales-otf @camfairchild @gztensor @sam0x17